Conversation
Previously, the ticklabelindex property itself was used in boolean tests which can be confusing.
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
Most of this works, but there are a few changes that need to be made. I'm happy to talk through how to do some of this.
| } | ||
| return minor; | ||
| }) | ||
| .toSorted((a, b) => a.value - b.value); |
There was a problem hiding this comment.
It turns out I was wrong before about esbuild. It won't transpile this method, so you'll have to use sort instead.
| .toSorted((a, b) => a.value - b.value); | |
| .slice() | |
| .sort((a, b) => a.value - b.value); |
| // for period label positioning when using `ticklabelindex`: | ||
| // for each tick in `allTicklabelVals` holds the neighboring period end tick | ||
| var periodEndTicks; |
There was a problem hiding this comment.
Instead of saving an array of end ticks for each tick, what if we save the end tick to each tick? Then you could check for an end tick on each tick and use a different code path. You'd also be able to get rid of the new arg in positionPeriodTicks.
| // This minor tick will be labeled instead of the major tick. | ||
| if (isPeriod) periodEndTicks = []; // for each minor tick at the start of a labeled period this will hold the neighboring period end tick. | ||
|
|
||
| const labelTickValsAscending = minorTickVals |
There was a problem hiding this comment.
What should happen if there's a major tick that doesn't coincide with a minor tick? Should that also be included?
| var isReversed = ax.range[0] > ax.range[1]; | ||
| var ticklabelIndex = (!ax.ticklabelindex || Lib.isArrayOrTypedArray(ax.ticklabelindex)) ? | ||
| ax.ticklabelindex : [ax.ticklabelindex]; | ||
| ax._useTicklabelIndex = ticklabelIndex != null && ticklabelIndex !== 0; |
There was a problem hiding this comment.
This is only used inside calcTicks, so it can just be a local variable.
| ax._useTicklabelIndex = ticklabelIndex != null && ticklabelIndex !== 0; | |
| let useTicklabelIndex = ticklabelIndex != null && ticklabelIndex !== 0; |
| majorTick.skipLabel = majorTick.skipLabel !== false; | ||
| }); |
There was a problem hiding this comment.
majorTick.skipLabel will always be true given the if check above. Let's move this out of the inner loop and assign the value directly.
| majorTick.skipLabel = majorTick.skipLabel !== false; | |
| }); | |
| }); | |
| // Skip the major tick label since the label moved to a minor tick | |
| majorTick.skipLabel = true; |
| if (labelIndex < largerTicks.length - 1) { | ||
| allTicklabelVals.push(largerTicks[labelIndex]); | ||
| if (isPeriod) periodEndTicks.push(largerTicks[labelIndex + 1]); | ||
| } |
There was a problem hiding this comment.
I understand why you're using length -1, but that means that you're going to lose the last label. You could run through the entire array and check that largerTicks[labelIndex + 1] is defined before pushing it. This logic would also need to change if you start adding the end tick directly to the tick itself.
camdecoster
changed the title
2 participants
Fixes #7876 by rewriting the way period label positioning works when using ticklabelindex.
Previously, the next labeled tick was used as the period end for the previous tick. This was problematic when using ticklabelindex, because sometimes the next labeled period is not necessarily adjacent to the last labeled period.
Now positionPeriodTicks can accept an array of periodEndTicks instead.